We might want to trigger the workflow only when the source code of ion-c has been changed.

on:
pull_reuqest: 
  paths:
        - 'ionc/*'

Good call, adding.

Nitpick: We might prefer to use a for loop to avoid repeated logic.

for test in nestedStruct nestedList sexp realWorldDataSchema01 realWorldDataSchema02 realWorldDataSchema03
do
java -jar $jar_file generate -S ${{env.data_size}} --input-ion-schema $schema_dir/${test}.isl testData/${test}.10n
done

Yea.. I had originally broken it out because what I had taken from either ion-python's, or ion-java's
workflow had -S 100 which resulted in some wildly different dataset sizes. I think the sexp.10n was 195bytes, which definitely wasn't large enough to overcome the normal runtime noise (5% diff ended up being ~195ns on my system). Once I saw what -S does, and realized what was going on, I didn't re-loop it. I'll tidy that up.

Could we pin to a specific commit instead of commit hash tag? Tags can be updated and re-pointed, so the code referenced by a tag can change. However the commit SHA represents the exact snapshot of the code at a point in time.

The latest version of actions/checkout is recommended. actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
Same suggestion to other actions/checkout call.

I'd like to know if alpha value can be adjusted based on the accuracy of regression-detection workflow?

Absolutely, alpha can be adjusted in order to fine-tune the null-hypothesis test. That is why it is exposed, so that we can adjust as needed. However, right now the results of the null-hypothesis test aren't being used because the p-value that is being calculated between revisions almost always determines the benchmarks to have statistically-significant differences.

I plan to spend a little more time looking into the test to see if it is appropriate for these benchmarks.

Thanks for the explanation.

Approve this PR with one question here: When the cpu_time_perc_diff > 0, it represents the candidates CPU run time is bigger than the baseline CPU run time right? If the value is bigger than the threshold value we will fail the workflow with regression detected.

Yes, exactly. The way the compare.py generates its output, we end up with data like this:

  {
    "name": "realWorldDataSchema03.10n_mean",
    "label": "",
    "measurements": [
      {
        "real_time": 12452149.371052643,
        "cpu_time": 12451057.894736838,
        "real_time_other": 12473477.750893278,
        "cpu_time_other": 12470339.46428573,
        "time": 0.0017128271758622316,
        "cpu": 0.0015485888598303227
      }
    ],
    "time_unit": "ns",
    "run_type": "aggregate",
    "aggregate_name": "mean",
    "utest": {}
  },

This represents the baseline (cpu_time) and candidate's (cpu_time_other) mean (of 20 runs) wallclock, and cpu time for the realWorldDataSetSchema03 test. The cpu field represents the relative difference ((candidate - baseline)/(baseline)) between the candidate and the baseline. Likewise, the time field represents the relative wallclock difference. Since the relative difference is a percentage of the baseline, I multiply it by 100 it so we can configure the threshold with whole values. Any positive value is a results of the candidate's time being larger than the baseline's.

The check step, passes the JSON output through that jq query. The JSON output from the compare.py, contains both the raw results from the candidate run, but also the comparisons to the baseline like I mentioned above. The jq query looks for any aggregate comparisons, for the mean, and gathers only results that exceed the threshold. If there are no values, the RESULTS variable will be an empty string, otherwise it will contain JSON that includes the name and CPU %diff for each failing run. The step then just checks to see if RESULTS is empty or not, when deciding if the step should fail or not. If it's not, it will also use jq to print out the name and %diff for each of the tests that exceed the threshold.

Hah.. typing this up made me realize I missed the aggregate_name field. I could make the jq query a little friendlier. I'll follow up with that in a later PR.

Thanks for the detailed explanation. Sounds good to me.